-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating github actions and requirements #257
Conversation
* Updating github actions * Update CI * add numpy cython install * Check brew list * Automake * gfortran * alias * environment variable * brew link * try again * one block * fix typo * indent * remove numpy and cython from pyproject * change jellis-> vallis and add python requirement * Using logger and adding isort * isort config * logger.info -> print in docsting * Adding wheels and pypi * Adding "build requirements" for scikit-sparse
Codecov Report
@@ Coverage Diff @@
## master #257 +/- ##
==========================================
+ Coverage 85.66% 85.97% +0.30%
==========================================
Files 12 12
Lines 2748 2737 -11
==========================================
- Hits 2354 2353 -1
+ Misses 394 384 -10
Continue to review full report at Codecov.
|
This looks great Justin. Thank you so much. Several things:
|
@vallis if you just want coverage percent as a check I added that in this PR directly as part of the tests. As for suitesparse I was unaware there were different builds of the main library itself. I thought it just had to do with whether or not a mkl numpy version was used to build scikit-sparse. Unfortunately I seems nearly impossible to get mkl-based numpy though pip. I think the best approach may be to leave as is for the pip install path and just use conda if you care about speed. Doesn't look like conda-forge does anything special for suitesparse How have you guys handled this in the past? |
try: | ||
import libstempo as t2 | ||
except ImportError: | ||
print("Ooh, no libstempo?") | ||
logger.warning("libstempo not installed. Will use PINT instead.") # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nanograv/enterprise These are not really needed if we have both libstempo
and PINT
as dependencies. Do we want to keep them both or have one as optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you changed them to logger
warnings. I think we could make the case either way. People only need one of the packages to run enterprise. I think the only dependency on either package right now is within enterprise.Pulsar
, so users can get a way with only using one. I guess that is a pretty rare situation for software. Maybe we just raise an ImportError
if neither are present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think there are 3 options:
- Don't require either as base dependencies and have a
pip install enterprise-pulsar[libstempo]
andpip install enterprise-pulsar[pint]
path. I don't really like this because it could lead to broken installs because the package won't really work without one or the other. - Choose one as the default and add the other as an extra. Kind of like this one but not sure which should be default
- Leave both as it is now. This is fine too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the whole Pulsar
function/class is a pretty bad design. I don't think I knew much about class polymorphism at the time. Its too much to refactor it for this PR but it could definitely be better.
@nanograv/enterprise I don't know how to turn off these checks above that aren't relevant... |
I think we can delete them from
Then there will be no patch coverage check. You are rerunning tests now, so I think those were the tests that weren't passing, is that correct? |
@Hazboun6 the tests all pass now but yes we could remove that patch coverage check. What I don't know how to get rid of are these They don't run on pull requests and I don't even know where "lint" comes from as its not a target Actually I think I see now. Those were the old targets but aren't there anymore |
Yeah, @jellis18 is this because GitHub Actions runs the old tests and the new tests in a PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jellis18 This all looks great. Thanks for doing the hard work of getting the requirements set up correctly.
@Hazboun6, fixed it |
This PR does the following:
isort
pre-commit. This will ensure that imports are always consistentprint
with loggers where appropriate__future__
since python 2.7 support is dropped.There are still a few things that will need to be taken care of down the line. First off, we are still installing libstempo from github and not from a pypi release. I've been working with @vallis on getting a pypi release ready so that will be along soon.
Next, scikit-sparse still requires
numpy
andcython
to be installed beforehand because they don't have the right build dependencies set. I made a PR to fix this but it may not get any attention (the repo hasn't been touched for a few years). Another option is to maybe fork it and use that for ourscikit-sparse
install (I already did this but it may be better to try to work through the scikit-sparse devs first).Lastly, all of these changes are mainly meant for the
pip
path. Once there is a version on pypi we can then make a conda-forge version.